Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(dashboard/pyroscope): integrate pyroscope database and dashboard #3059

Closed
wants to merge 4 commits into from

Conversation

cpeliciari
Copy link
Contributor

  • Added Pyroscope database configuration in Grafana
  • Included Pyroscope service in the docker-compose setup
  • Created a new dashboard named "go-libp2p Profiling" for visualizing profiling data

image

- Added Pyroscope database configuration in Grafana
- Included Pyroscope service in the docker-compose setup
- Created a new dashboard named "go-libp2p Profiling" for visualizing profiling data
@sukunrt
Copy link
Member

sukunrt commented Nov 25, 2024

Thanks for the PR @cpeliciari. I'll take a look at this some time this week. I just need to think about whether we have the bandwidth to support and maintain this. cc @MarcoPolo

@sukunrt sukunrt self-assigned this Nov 25, 2024
@p-shahi
Copy link
Member

p-shahi commented Dec 2, 2024

@cpeliciari this seems useful; echo Sukun's concern about maintenance. How did you generate the json file, and could you add some more info into the readme about Pyroscope?

@MarcoPolo
Copy link
Collaborator

This is pretty cool! This seems pretty well separated from the rest of the metrics/dashboards. Is there a reason to want it merged into this repo? I think if we had many users all wanting to reference this exact dashboard that would be a good case. If it's just a nice demo or proof-of-concept, maybe we could put some information on docs.libp2p.io?

@cpeliciari
Copy link
Contributor Author

When I came across the dashboard, I thought it was something to help developers visualize what is happening with libp2p. I had some consumption issues with the library when I was running some tests.

@p-shahi
Copy link
Member

p-shahi commented Dec 30, 2024

@aschmahmann @sukunrt and I discussed this in the maintainers call last week, we are on board with merging this since it's a good contribution and can be useful. The consensus was that it doesn't seem like too much of a maintenance overhead and merging it here would be more useful that in another repo. In case, there will be too much overhead required to upkeep pyroscope, we'll remove it later. Thanks @cpeliciari

@sukunrt let's review and merge?

Copy link
Member

@sukunrt sukunrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for getting to this so late. I was recovering from an accident.

I have changed my mind regarding including this in the repo(cc @p-shahi @aschmahmann). Integrating pyroscope has nothing to do with go-libp2p. The steps mentioned here will work the same with any golang application. For prometheus we have metrics built in and we have this elaborate setup so users can explore the dashboards quickly.

I have reviewed this in case we do decide to include this in the repo.

Comment on lines +40 to +42
// replace this with the address of pyroscope server
ServerAddress: "http://127.0.0.1:4040",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this with the address of the server that we setup with docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can use docker DNS.

Comment on lines +37 to +38
pyroscope.Start(pyroscope.Config{
ApplicationName: "simple.golang.app",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to go in the goroutine above so that users can copy paste this block.

This will still not work because the go.mod doesn't have pyroscope, so we should add a line regarding it somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no need to start it in a goroutine, since pyroscope.Start already does that.
I can create an example and include pyroscope. I can even create a tracing example that includes pubsub, DHT, etc., to provide an example of how to do tracing.

@cpeliciari
Copy link
Contributor Author

Apologies for getting to this so late. I was recovering from an accident.

I have changed my mind regarding including this in the repo(cc @p-shahi @aschmahmann). Integrating pyroscope has nothing to do with go-libp2p. The steps mentioned here will work the same with any golang application. For prometheus we have metrics built in and we have this elaborate setup so users can explore the dashboards quickly.

I have reviewed this in case we do decide to include this in the repo.

No problem, I can see how dedicated you all are to the project. That was just a way for the developers to check the usage of the library. And I already thank you for working so hard on libp2p.

@p-shahi
Copy link
Member

p-shahi commented Feb 10, 2025

Thanks for the update @sukunrt - let's close this PR then @cpeliciari ? Thanks for the contribution nonetheless 🙏

@cpeliciari cpeliciari requested a review from sukunrt February 10, 2025 16:56
@cpeliciari cpeliciari closed this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants